-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Zend: Deprecate __sleep() and __wakeup() #19435
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
a9b99c2
to
e21a296
Compare
e21a296
to
8653dd3
Compare
@@ -110,6 +110,10 @@ $db = MySQLPDOTest::factory(); | |||
$db->exec('DROP TABLE IF EXISTS test_stmt_fetchserialize_fetch_class'); | |||
?> | |||
--EXPECTF-- | |||
Deprecated: The __sleep() serialization hook has been deprecated. Implement __serialize() instead (or in addition, if support for old PHP versions is necessary) in %s on line %d |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can these tests not be rewritten to use the new methods?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, because those tests are for the deprecated PDO::FETCH_SERIALIZE
option which relies on the deprecated Serialize
interface methods.
That's a lot of tests to be updated. I have been looking into this more and checking the actual conversion in real code. This might actually suck for users a bit because in this case we basically tell them to add more code for no benefit. For example compare those two: class User {
public function __construct(
public readonly string $id,
public readonly string $email,
public readonly DateTime $createdAt
) {}
public function __sleep(): array {
return ['id', 'email', 'createdAt'];
}
public function __wakeup(): void {
if (!filter_var($this->email, FILTER_VALIDATE_EMAIL)) {
throw new InvalidArgumentException('Invalid email format');
}
if (empty($this->id)) {
throw new InvalidArgumentException('ID cannot be empty');
}
}
} The conversion would be like this: class User {
public function __construct(
public readonly string $id,
public readonly string $email,
public readonly DateTime $createdAt
) {}
public function __serialize(): array {
return [
'id' => $this->id,
'email' => $this->email,
'createdAt' => $this->createdAt,
];
}
public function __unserialize(array $data): void {
if (!filter_var($data['email'], FILTER_VALIDATE_EMAIL)) {
throw new InvalidArgumentException('Invalid email format');
}
if (empty($data['id'])) {
throw new InvalidArgumentException('ID cannot be empty');
}
$this->id = $data['id'];
$this->email = $data['email'];
$this->createdAt = $data['createdAt'];
}
} Now imagine you have class with 50 properties. Why do we need to force users to add this extra code? On top of that this is also slower in terms of perf. I did a little bit of testing and just with 3 props, wake up was most of the time faster so with 50+ props, it might be even visible but probably not something major to worry about. The way how I see it is that __unserialize is not a direct replacement for __wakeup but more a method that can be used if different mapping should be used. But it doesn't seem like the use case that everyone has so for many users it might just mean that @Girgias Have you actually considered all of this before proposing the deprecation? It passed by closest possible margin (1vote) and I guess some of the voters might have not been aware what's required (honestly I haven't realised it until now and I have actually worked with it in past - I voted no because I thought it does not harm to keep it but in reality it is actually a burden to do the migration and it results in worse code). I think if the RFC showed some examples of needed migration and also mentioned potential perf impact, it wouldn't pass. |
Btw the examples are a bit stripped off as above wouldn't need to use __sleep and __serialze (there should be some extra props added but it's just to show a minimal conversion without extra props in the class. |
Not sure why am receiving this email. |
RFC: https://wiki.php.net/rfc/deprecations_php_8_5#deprecate_the_sleep_and_wakeup_magic_methods